Skip to content

unixgram: Make fd non-blocking#402

Merged
slp merged 1 commit into
containers:mainfrom
jakecorrenti:fix-unixgram
Sep 1, 2025
Merged

unixgram: Make fd non-blocking#402
slp merged 1 commit into
containers:mainfrom
jakecorrenti:fix-unixgram

Conversation

@jakecorrenti
Copy link
Copy Markdown
Member

When creating unixgram backend from existing fd we need to make it non-blocking so the user does not have to care about this implementation detail. Disabling SIGPIPE is needed on the existing fd for the same reason.

Fix by moving these steps from open() to new().

Authored-by: Nir Soffer nirsof@gmail.com

Comment thread src/devices/src/virtio/net/unixgram.rs Outdated
send(fd, &VFKIT_MAGIC, MsgFlags::empty()).map_err(ConnectError::SendingMagic)?;
}

// macOS forces us to do this here instead of just using SockFlag::SOCK_NONBLOCK above.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment was good in open() since we created the socket above. In new() we need another comment explaining that we handle socket from 2 sources - user socket and socket created in open. The comment in my patch tried to explain this by maybe it should be improved:

// For existing socket we must make the socket non-blocking here. When
// opening the socket we can create it non-blocking but not on macOS.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've opted for simplifying the comment, since in that context doesn't make sense to talk about macOS.

@nirs
Copy link
Copy Markdown
Contributor

nirs commented Aug 30, 2025

@jakecorrenti added issue #403

@nirs
Copy link
Copy Markdown
Contributor

nirs commented Aug 30, 2025

Tests, this fixes the issue:

Using krunkit PR adding unixgram device:
libkrun/krunkit#63

And vmnet-helper PR for supporting unixgram device:
nirs/vmnet-helper#92

VM using fd=

% ./example server --driver krunkit --connection fd
Starting vmnet-helper for 'server' with interface id 'bdc7b3e0-8594-4814-aa25-05187ad2a36e'
Creating cloud-init iso '/Users/nir/.vmnet-helper/vms/server/cidata.iso'
Starting 'krunkit' virtual machine 'server' with mac address '92:c9:52:b7:6c:08'
Virtual machine IP address: 192.168.105.2

The krunkit command:

% ps | grep krunkit | grep fd=                     
 4769 ttys007    0:04.34 krunkit --memory=1024 --cpus=1 --restful-uri=none:// --device=virtio-blk,path=/Users/nir/.vmnet-helper/vms/server/disk.img --device=virtio-blk,path=/Users/nir/.vmnet-helper/vms/server/cidata.iso --device=virtio-serial,logFilePath=/Users/nir/.vmnet-helper/vms/server/serial.log --krun-log-level=3 --device=virtio-net,type=unixgram,fd=3,mac=92:c9:52:b7:6c:08,offloading=off

iperf3 run:

% iperf3 -c 192.168.105.2   
Connecting to host 192.168.105.2, port 5201
[  5] local 192.168.105.1 port 55110 connected to 192.168.105.2 port 5201
[ ID] Interval           Transfer     Bitrate
[  5]   0.00-1.01   sec  1.04 GBytes  8.92 Gbits/sec                  
[  5]   1.01-2.01   sec  1.06 GBytes  9.09 Gbits/sec                  
[  5]   2.01-3.01   sec  1.06 GBytes  9.10 Gbits/sec                  
[  5]   3.01-4.01   sec  1.05 GBytes  9.01 Gbits/sec                  
[  5]   4.01-5.01   sec  1.05 GBytes  9.06 Gbits/sec                  
[  5]   5.01-6.01   sec  1.05 GBytes  8.99 Gbits/sec                  
[  5]   6.01-7.01   sec  1.06 GBytes  9.12 Gbits/sec                  
[  5]   7.01-8.01   sec  1.05 GBytes  9.06 Gbits/sec                  
[  5]   8.01-9.01   sec  1.05 GBytes  9.05 Gbits/sec                  
[  5]   9.01-10.01  sec  1.05 GBytes  9.04 Gbits/sec                  
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval           Transfer     Bitrate
[  5]   0.00-10.01  sec  10.5 GBytes  9.04 Gbits/sec                  sender
[  5]   0.00-10.01  sec  10.5 GBytes  9.04 Gbits/sec                  receiver

VM using path=

% ./example server --driver krunkit --connection socket
Starting vmnet-helper for 'server' with interface id 'bdc7b3e0-8594-4814-aa25-05187ad2a36e'
Creating cloud-init iso '/Users/nir/.vmnet-helper/vms/server/cidata.iso'
Starting 'krunkit' virtual machine 'server' with mac address '92:c9:52:b7:6c:08'
Virtual machine IP address: 192.168.105.2
% ps | grep krunkit | grep path=  
 4798 ttys007    0:03.20 krunkit --memory=1024 --cpus=1 --restful-uri=none:// --device=virtio-blk,path=/Users/nir/.vmnet-helper/vms/server/disk.img --device=virtio-blk,path=/Users/nir/.vmnet-helper/vms/server/cidata.iso --device=virtio-serial,logFilePath=/Users/nir/.vmnet-helper/vms/server/serial.log --krun-log-level=3 --device=virtio-net,type=unixgram,path=/Users/nir/.vmnet-helper/vms/server/vmnet.sock,mac=92:c9:52:b7:6c:08,offloading=off

iperf3 run:

% iperf3 -c 192.168.105.2
Connecting to host 192.168.105.2, port 5201
[  5] local 192.168.105.1 port 55117 connected to 192.168.105.2 port 5201
[ ID] Interval           Transfer     Bitrate
[  5]   0.00-1.00   sec  1.03 GBytes  8.78 Gbits/sec                  
[  5]   1.00-2.01   sec  1.05 GBytes  9.00 Gbits/sec                  
[  5]   2.01-3.01   sec  1.03 GBytes  8.84 Gbits/sec                  
[  5]   3.01-4.01   sec  1.05 GBytes  8.99 Gbits/sec                  
[  5]   4.01-5.00   sec  1.05 GBytes  9.05 Gbits/sec                  
[  5]   5.00-6.00   sec  1.06 GBytes  9.12 Gbits/sec                  
[  5]   6.00-7.01   sec  1.05 GBytes  9.01 Gbits/sec                  
[  5]   7.01-8.00   sec  1.05 GBytes  8.99 Gbits/sec                  
[  5]   8.00-9.00   sec  1.06 GBytes  9.08 Gbits/sec                  
[  5]   9.00-10.00  sec  1.05 GBytes  9.04 Gbits/sec                  
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval           Transfer     Bitrate
[  5]   0.00-10.00  sec  10.5 GBytes  8.99 Gbits/sec                  sender
[  5]   0.00-10.00  sec  10.5 GBytes  8.99 Gbits/sec                  receiver

Copy link
Copy Markdown
Contributor

@nirs nirs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comments can be improved later.

nirs added a commit to nirs/vmnet-helper that referenced this pull request Aug 30, 2025
Latest krunkit can use a file descriptor instead of unix socket. Update
the krunkit command to support this. Since krunkit behaves like vfkit
and qemu, we can remove the special "auto" connection.

Depends on:
- libkrun/krunkit#63
- containers/libkrun#402
nirs added a commit to nirs/vmnet-helper that referenced this pull request Aug 30, 2025
Previously we use "auto", "on", "off" to enable offloading automatically
for krunkit since it was always required. Now that this is an optional
features we can simplify to boolean flag.

Depends on:
- libkrun/krunkit#63
- containers/libkrun#402
@jakecorrenti jakecorrenti marked this pull request as ready for review September 1, 2025 02:26
@jakecorrenti jakecorrenti changed the title WIP: unixgram: Make fd non-blocking unixgram: Make fd non-blocking Sep 1, 2025
@slp
Copy link
Copy Markdown
Collaborator

slp commented Sep 1, 2025

The comment for krun_add_net_unixgram in libkrun.h states that:

If using "fd", the socket must be already initialized and configured as the userspace network proxy requires.

In my head, that also meant library users would need to configure the socket as non-blocking before calling the function. But it's true that, strictly speaking, making it non-blocking is our requirement, not the userspace network proxy requirement, so I guess it's reasonable doing this ourselves...

About this PR in particular, we need to make it non-blocking on both new and open, as otherwise we're fixing one but breaking the other.

@slp
Copy link
Copy Markdown
Collaborator

slp commented Sep 1, 2025

About this PR in particular, we need to make it non-blocking on both new and open, as otherwise we're fixing one but breaking the other.

Correcting myself, open ends calling new, so this should work just fine.

When creating unixgram backend from existing fd we need to make it
non-blocking so the user does not have to care about this implementation
detail. Disabling SIGPIPE is needed on the existing fd for the same
reason.

Fix by moving these steps from open() to new().

Authored-by: Nir Soffer <nirsof@gmail.com>
Signed-off-by: Nir Soffer <nirsof@gmail.com>
Signed-off-by: Jake Correnti <jakecorrenti+github@proton.me>
Signed-off-by: Sergio Lopez <slp@redhat.com>
Copy link
Copy Markdown
Collaborator

@slp slp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @nirs and @jakecorrenti

@slp slp merged commit f617f3d into containers:main Sep 1, 2025
6 checks passed
slp added a commit to slp/libkrun that referenced this pull request Sep 1, 2025
Set up the stage for a quick bug-fix release including a patch
fixing network configuration with unixgram sockets (containers#402).

Signed-off-by: Sergio Lopez <slp@redhat.com>
slp added a commit that referenced this pull request Sep 1, 2025
Set up the stage for a quick bug-fix release including a patch
fixing network configuration with unixgram sockets (#402).

Signed-off-by: Sergio Lopez <slp@redhat.com>
nirs added a commit to nirs/vmnet-helper that referenced this pull request Sep 4, 2025
Latest krunkit can use a file descriptor instead of unix socket. Update
the krunkit command to support this. Since krunkit behaves like vfkit
and qemu, we can remove the special "auto" connection.

Depends on:
- libkrun/krunkit#63
- containers/libkrun#402
nirs added a commit to nirs/vmnet-helper that referenced this pull request Sep 4, 2025
Previously we use "auto", "on", "off" to enable offloading automatically
for krunkit since it was always required. Now that this is an optional
features we can simplify to boolean flag.

Depends on:
- libkrun/krunkit#63
- containers/libkrun#402
mtjhrc pushed a commit to mtjhrc/libkrun that referenced this pull request Nov 10, 2025
Set up the stage for a quick bug-fix release including a patch
fixing network configuration with unixgram sockets (containers#402).

Signed-off-by: Sergio Lopez <slp@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants